-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added functors to make working with parameters of bijectors easier #143
Conversation
Codecov Report
@@ Coverage Diff @@
## master #143 +/- ##
=======================================
Coverage 54.98% 54.98%
=======================================
Files 27 27
Lines 1726 1726
=======================================
Hits 949 949
Misses 777 777
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slick, easy, no comments. LGTM once you bump the version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to add some tests?
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Got a suggestion? Not sure how useful it is to add a test to check if, say, |
For instance, in KernelFunctions we check that |
But this has to be done on a case-by-base basis, right? Not quite clear how we can do this in general. |
Maybe one easy test would be to just check whether the parameters in |
Super sophisticated tests for this are probably not worth it -- we just need to check some very basic functionality. Functors.jl bears a lot more of the onus for testing all the cases. |
Hmm you mean that it has to be tested for every bijector for which we define |
Yeah, exactly. Well, it's a "problem" because currently we just list a bunch of bijectors which are then iterated through and tested. So we'd have to add a new suite for which we go through a bunch of different combinations. Now, that's of course possible. It's just that I'm uncertain how useful it is. Imagine a PR being made for a new bijector. The only way a test using If we already have an added test, what are the possible ways they can fail?
The first should never happen. The second should be caught in Functors.jl rather than here. Are there more ways it could fail? Btw, I'm well aware that I could have added the tests in the time it took me to write this comment 😅 So it's not about whether or not I'm bothered to do it, it's just that if it doesn't actually add anything to the test-suite, IMO it shouldn't be there. I've recently made some efforts towards making it possible to test a new Bijector by simply calling a function like Do prove me wrong though! I might be missing something. |
IMO the main point of adding tests for
Without tests, the first case might happen silently when refactoring the code. Custom implementations of I like the xs, re = functor(my_bijector)
@test my_bijector == re(xs) |
Ah, testing the reconstruction is a great idea 👍 When I last used |
Closed in favour of #162 |
The people have spoken: they want functors, so here there be functors.
Also, it makes it easy to reconstruct
Bijector
with different parameters, e.g.